Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cloud-api-adaptor: Support podman in Build.sh #1798

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

cloud-api-adaptor: Support podman in Build.sh #1798

wants to merge 1 commit into from

Conversation

davidhadas
Copy link
Member

Update build.sh to support podman
podman does not support the --push flag

@davidhadas
Copy link
Member Author

Please test with docker before merging. Was tested with podman.

@beraldoleal
Copy link
Member

Please, do not merge this PR before 0.8.2 release is cut.

stevenhorsman
stevenhorsman approved these changes Apr 26, 2024
View reviewed changes
Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this a quick try locally and it seemed to work for me. Thanks @davidhadas

beraldoleal
beraldoleal reviewed Apr 26, 2024
View reviewed changes
Copy link
Member

@beraldoleal beraldoleal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @davidhadas I have tested and lgtm, thanks.

Just one small comment for you.

src/cloud-api-adaptor/hack/build.sh Outdated Show resolved Hide resolved
@stevenhorsman stevenhorsman added the test_e2e_libvirt Run Libvirt e2e tests label Apr 30, 2024
beraldoleal
beraldoleal reviewed May 1, 2024
View reviewed changes
Copy link
Member

@beraldoleal beraldoleal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @davidhadas this lgtm, but we still have one test failing:

https://github.com/confidential-containers/cloud-api-adaptor/actions/runs/8900345375/job/24441704047?pr=1798#step:10:123

Do you mind taking a look?

@davidhadas
Copy link
Member Author

Hi @davidhadas this lgtm, but we still have one test failing:

https://github.com/confidential-containers/cloud-api-adaptor/actions/runs/8900345375/job/24441704047?pr=1798#step:10:123

Do you mind taking a look?

Can we run the tests again to see if this is a transient test issue or somehow related to the change we made?
For some reason I do not see a way to do that.

@beraldoleal
Copy link
Member

Hi @davidhadas this lgtm, but we still have one test failing:
https://github.com/confidential-containers/cloud-api-adaptor/actions/runs/8900345375/job/24441704047?pr=1798#step:10:123
Do you mind taking a look?

Can we run the tests again to see if this is a transient test issue or somehow related to the change we made? For some reason I do not see a way to do that.

It seems to be related:

~/work/cloud-api-adaptor/cloud-api-adaptor/src ~/work/cloud-api-adaptor/cloud-api-adaptor/src/cloud-api-adaptor
ERROR: docker exporter does not currently support exporting manifest lists

In any case I re triggered the failed jobs.

@stevenhorsman
Copy link
Member

Yes - I seems related as the PR adds in the --load command that seems linked to the error: docker/buildx#59

@davidhadas
Copy link
Member Author

davidhadas commented May 2, 2024 •
edited
Loading

Does it make sense to keep the changes for single arch and revert it to the multi-arch?
Can I assume that multi-arch is typically done for release purposes? If so, this can allow developers use podman and at the same time will still require that docker be used instead when doing a release.
(Until such time that an equivalent podman support of --push is added)

If this does not make sense, maybe we should drop this PR for now.

@stevenhorsman
Copy link
Member

Does it make sense to keep the changes for single arch and revert it to the multi-arch?
Can I assume that multi-arch is typically done for release purposes? If so, this can allow developers use podman and at the same time will still require that docker be used instead when doing a release.
(Until such time that an equivalent podman support of --push is added)

I think it is fair to say that developers are most likely to build single arch (an example case in point being that I did exactly that when I tested this) and the CI process does multi-arch. I'm personally a little bit reluctant to add complexity in adding two different ways, but if it can be done fairly simply then I'd be open to it.

@davidhadas
Copy link
Member Author

Tests still fail - not sure why.

@stevenhorsman
Copy link
Member

Tests still fail - not sure why.

I was able to reproduce this locally using your branch:

# cd src/cloud-api-adaptor && ARCHES=linux/amd64,linux/s390x,linux/ppc64le RELEASE_BUILD=false DEV_TAGS=test make image registry=quay.io/stevenhorsman
bash: line 1: go: command not found
COMMIT=ae85b24817aca5aae31dadf83f99bd64d9ad7e68 VERSION=v0.8.1-alpha.1-dev YQ_VERSION=v4.35.1 YQ_CHECKSUM="sha256:bd695a6513f1196aeda17b174a15e9c351843fb1cef5f9be0af170f2dd744f08" hack/build.sh -i
~/go/src/github.com/confidential-containers/cloud-api-adaptor/src ~/go/src/github.com/confidential-containers/cloud-api-adaptor/src/cloud-api-adaptor
[+] Building 0.0s (0/0)                                                                                                             docker:default
ERROR: docker exporter does not currently support exporting manifest lists
make: *** [Makefile:113: image] Error 1

@davidhadas
Copy link
Member Author

I was able to reproduce this locally using your branch:

# cd src/cloud-api-adaptor && ARCHES=linux/amd64,linux/s390x,linux/ppc64le RELEASE_BUILD=false DEV_TAGS=test make image registry=quay.io/stevenhorsman
bash: line 1: go: command not found
COMMIT=ae85b24817aca5aae31dadf83f99bd64d9ad7e68 VERSION=v0.8.1-alpha.1-dev YQ_VERSION=v4.35.1 YQ_CHECKSUM="sha256:bd695a6513f1196aeda17b174a15e9c351843fb1cef5f9be0af170f2dd744f08" hack/build.sh -i
~/go/src/github.com/confidential-containers/cloud-api-adaptor/src ~/go/src/github.com/confidential-containers/cloud-api-adaptor/src/cloud-api-adaptor
[+] Building 0.0s (0/0)                                                                                                             docker:default
ERROR: docker exporter does not currently support exporting manifest lists
make: *** [Makefile:113: image] Error 1

Should you not be using make image-with-arch ?

@davidhadas
Copy link
Member Author

Somehow the CI tests use hack/build.sh -i (i.e. make image) even when building multiple arch images.

This results in multi arch reaching the build_caa_payload_image() function which can only handle a single image (as it is using --load).

@davidhadas
cloud-api-adaptor: Support podman in Build.sh
a2c4d4b 
Update build.sh to support `podman`

`podman` does not support the --push flag
This change only apply to building caa images in a development
enviroment where a single arch is used.

`podman` is not supported for building multi-arch.
Use Docker instead.

Signed-off-by: David Hadas <[email protected]>
@stevenhorsman
Copy link
Member

The e2e_run_all workflow uses caa_build_and_push.yaml:

cloud-api-adaptor/.github/workflows/e2e_run_all.yaml

Line 83 in 0db87d0

uses: ./.github/workflows/caa_build_and_push.yaml
which runs make image whereas the publish image on push and release workflows use the multi-arch version I think (which I think for performance reasons builds all the images for a single arch and then makes a manifest for them at the end. I don't know why it is using different code, it might be possible to unify it.

@stevenhorsman stevenhorsman self-requested a review May 2, 2024 11:55
@davidhadas
Copy link
Member Author

/hold till we figure this out.

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 5, 2024

This PR has been opened without with no activity for 90 days. Comment on the issue otherwise it will be closed in 7 days

@github-actions github-actions bot added the Stale label Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beraldoleal beraldoleal beraldoleal left review comments

@stevenhorsman stevenhorsman Awaiting requested review from stevenhorsman

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
Stale test_e2e_libvirt Run Libvirt e2e tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davidhadas @beraldoleal @stevenhorsman